-
Notifications
You must be signed in to change notification settings - Fork 566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address deprecations in MP metrics for 3.x #4500
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ig resolution of gRPC metrics integration test beans
…oc, style clean-up
tjquinno
requested review from
ljnelson,
aseovic,
thegridman,
spericas and
tomas-langer
July 8, 2022 23:19
…JAX-RS services, instead inject a test bean and invoke annotated methods to test interceptors
tomas-langer
requested changes
Jul 11, 2022
...ain/java/io/helidon/microprofile/grpc/metrics/GrpcMetricAnnotationDiscoveryObserverImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/io/helidon/microprofile/grpc/metrics/GrpcMetricAnnotationDiscoveryObserverImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/io/helidon/microprofile/grpc/metrics/GrpcMetricAnnotationDiscoveryObserverImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/io/helidon/microprofile/grpc/metrics/GrpcMetricAnnotationDiscoveryObserverImpl.java
Outdated
Show resolved
Hide resolved
microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/InterceptorSimplyTimed.java
Outdated
Show resolved
Hide resolved
microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/InterceptorSimplyTimed.java
Outdated
Show resolved
Hide resolved
...ile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricAnnotationDiscoveryImpl.java
Outdated
Show resolved
Hide resolved
...ile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricAnnotationDiscoveryImpl.java
Outdated
Show resolved
Hide resolved
.../integration/mp-grpc/src/test/java/io/helidon/microprofile/grpc/server/InterceptorsTest.java
Outdated
Show resolved
Hide resolved
tomas-langer
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #4371
Background
The gRPC MP metrics component performs special handling for methods which are effectively annotated with
@GrpcMethod
and also bear a metrics annotation:In previous releases
During
ProcessAnnotatedType
processing, the gRPC MP metrics CDI extension removed the metrics annotation from methods with both@GrpcMethod
and a metrics annotation to prevent the normal interceptor binding. This not only prevented the normal metrics interceptor behavior (which is what gRPC really wanted), but it also burdened gRPC with registering the correct metrics with the correct metadata and name; this is because the metrics CDI extension no longer saw the metrics annotations on those gRPC methods and so could not register those metrics itself.To do this, gRPC MP metrics invoked several methods in and used data structures from MP metrics that should not be exposed, methods and data structures that we had marked as deprecated (to discourage users from relying on them and to remove them once we got around to refactoring things).
Redesign in 3.0.0
Note that these changes are intended for--and tuned to--usage from our own gRPC code. Although they are not intended for use from general user code we cannot prevent that. As a result there are no doc updates (other than JavaDoc) describing the new public interfaces.
Remove the hard binding of our metrics interceptors to the MicroProfile Metrics annotations
Helidon MP metrics has always had an interceptor for each different MP metric annotation. Previously, these were bound directly, by declaration, to the corresponding MP metric annotation.
This PR introduces separate annotations for the interceptor bindings.
The metrics CDI extension code now programmatically adds the correct interceptor binding annotation to each constructor and method if all observers agree that using the normal interceptor is OK. (See below for more about "all observers.")
Clean up the contract between gRPC and metrics
public
and@Deprecated
code.New interface in
io.helidon.microprofile.metrics
:MetricAnnotationDiscovery
Exposes information about the discovery of a metrics annotation as applied to an executable (constructor or method). Also allows an observer (see below) to:
In the new package
io.helidon.microprofile.metrics.spi
:MetricAnnotationDiscoveryObserver
- implementations are invoked with aMetricAnnotationDiscovery
object for each discovery of a metrics annotation which applies to an executable.MetricRegistrationObserver
- implementations are notified as each metric inspired by an annotation is registered. (There is no special "event" object here; the SPI method's parameters are the associated discovery and theMetadata
,MetricID
, andMetric
objects for the just-registered metric.)Change the MP metrics CDI extension
The metrics CDI extension now:
Modify the gRPC code
Implement the discovery interface to record discoveries and suppress the normal interceptor binding if appropriate, or deactivate the discovery if needed. These decisions are governed by gRPC rules that I've carried over from the old code.
Implement the registration interface to record metrics information gRPC needs for its own interceptor.
Remove the gRPC metrics CDI extension. It is no longer needed.
Update an important test to use
@HelidonTest
in place of the direct use of the Weld API.5.Because the metrics CDI extension now creates all metrics, even gRPC ones, and notifies enrolled observers, the test needs to have the CDI container and the metrics CDI extension running. Just starting the server using
@HelidonTest
caused failures in the gRPC server extension because of the way some test classes are designed to exercise specific behavior. I got things to work using@HelidonTest
and@DisableDiscovery
and explicitly adding the CDI extensions required for the test--which does not include the gRPC server CDI extension.Getting to this point was a bit challenging and revealed either:
The two gRPC experts are both out of the office for several days, so the updated
MetricsConfigurerTest
class in this PR is not as robust as it might be, but it works correctly. We might revisit this after the gRPC team weighs in. Any changes would not change the existing public interface or behavior of Helidon so we could improve this in a future dot release.Misc. Notes
I tried to purturb the general structure of the code as little as possible. There might have been ways to consolidate some of the data structures which
MetricsCdiExtension
manages internally, but that would have been considerably more work without much payoff. We are likely to undergo a significant rewrite of metrics for MP Metrics 5.0 and time was short enough as it was.